-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use a built-in JS function localeCompare()
to compare strings
#1141
use a built-in JS function localeCompare()
to compare strings
#1141
Conversation
@elzody Thank you. Changes undone for files
Side question: What's your NC setup for development and testing?Lately I've been having annoying misbehavior in my local NC developer instance I had spent a few hours to setup... So I've started thinking I should scrap it and go the docker-compose way. I'd appreciate if you guys @elzody @enjeck could suggest me an all-ready docker-compose.yml file as well as any steps to set it up... if you have such knowledge. (Sure, I've seen there are lots of images in nextcloud/docker-ci repository, but I can't wrap my head around it all on my own...) |
@kirisakow My setup, believe it or not, is also a bit messy. I need to take the time to improve the workflow a bit, but generally I use the standalone container and just bind mount the directories of the apps I am testing/developing for into the container. You can find more info about the standalone containers here, and a sample of my Docker compose file is like so:
Isn't the best, but works for me. The only part of the compose file I left out was that related to the Collabora container, not sure if you are gonna be using that at all. I will give another review here when I get the chance :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave it a test, and it works nicely. Looks much more readable than the weird ternary operators before.
I am not quite sure what is up with the DCO check failing, but I suspect it has something to do with the documentation
thing included in your first commit. Try doing an interactive rebase and see if you can alter that message to remove it, and we will see if that fixes it
Signed-off-by: Kiril Isakov <kirix.isakov@gmail.com>
Signed-off-by: Kiril Isakov <kirix.isakov@gmail.com>
b255c47
to
5eef3a3
Compare
You are right. My original commit message was split into 3 lines (with the URL on the 2nd line), but DCO seemingly wants no more than one newline break, with the sign-off being on the 2nd line. 🤷 |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
This PR is a proposal for using a built-in JavaScript function
localeCompare()
for strings sorting. If you read the documentation, you'll see it does exactly what was previously implemented by hand.Pros:
it is a built-in function of the JavaScript API;
s1.localeCompare(s2, 'your-locale')
looks cleaner and more legible than nested ternary operators that used to be there;it is more straightforward at handling numeric strings: no need to
parseInt(s)
orparseFloat(s)
before comparison, justs1.localeCompare(s2, 'your-locale', {numeric: true})
;